-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(blockifier): update assert_actual_fee_in_bounds and add test #1118
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1118 +/- ##
==========================================
- Coverage 74.18% 70.62% -3.56%
==========================================
Files 359 88 -271
Lines 36240 11365 -24875
Branches 36240 11365 -24875
==========================================
- Hits 26886 8027 -18859
+ Misses 7220 2960 -4260
+ Partials 2134 378 -1756
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware)
crates/blockifier/src/transaction/account_transactions_test.rs
line 190 at r1 (raw file):
#[case::positive_case_deprecated_tx(true, true)] #[case::positive_case_new_tx(true, false)] #[should_panic]
can you add the expected message?
https://doc.rust-lang.org/rust-by-example/testing/unit_testing.html#:~:text=This%20attribute%20accepts%20optional%20parameter%20expected%20%3D%20with%20the%20text%20of%20the%20panic%20message.
Code quote:
#[should_panic]
crates/blockifier/src/transaction/account_transactions_test.rs
line 202 at r1 (raw file):
if deprecated_tx { let tx = account_invoke_tx(invoke_tx_args! { max_fee: Fee(100),
please store this in a variable
Code quote:
100
crates/blockifier/src/transaction/account_transactions_test.rs
line 228 at r1 (raw file):
let context = Arc::new(block_context.to_tx_context(&tx)); AccountTransaction::assert_actual_fee_in_bounds(&context, Fee(actual_fee)); }
IMO this is more readable - less magic numbers, more readable loop. non-blocking
Suggestion:
// All resources.
let l1_gas = ResourceBounds { max_amount: 2, max_price_per_unit: 3 };
let l2_gas = ResourceBounds { max_amount: 4, max_price_per_unit: 5 };
let l1_data_gas = ResourceBounds { max_amount: 6, max_price_per_unit: 7 };
let all_resource_bounds =
ValidResourceBounds::AllResources(AllResourceBounds { l1_gas, l2_gas, l1_data_gas });
let all_resource_fee = l1_gas.max_amount * l1_gas.max_price_per_unit
+ l2_gas.max_amount * l2_gas.max_price_per_unit
+ l1_data_gas.max_amount * l1_data_gas.max_price_per_unit
+ actual_fee_offset;
// L1 resources.
let l1_resource_bounds = ValidResourceBounds::L1Gas(l1_gas);
let l1_resource_fee = l1_gas.max_amount * l1_gas.max_price_per_unit + actual_fee_offset;
for (bounds, actual_fee) in
[(all_resource_bounds, all_resource_fee), (l1_resource_bounds, l1_resource_fee)]
{
let tx = account_invoke_tx(invoke_tx_args! {
resource_bounds: bounds,
version: TransactionVersion::THREE,
});
let context = Arc::new(block_context.to_tx_context(&tx));
AccountTransaction::assert_actual_fee_in_bounds(&context, Fee(actual_fee));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware)
crates/blockifier/src/transaction/account_transaction.rs
line 472 at r1 (raw file):
match &tx_context.tx_info { TransactionInfo::Current(context) => { let max_fee = context.resource_bounds.max_possible_fee();
Talking via slack to make sure I understand
Code quote:
let max_fee = context.resource_bounds.max_possible_fee();
Signed-off-by: Dori Medini <dori@starkware.co>
b4c059b
to
deab7b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @amosStarkware)
crates/blockifier/src/transaction/account_transaction.rs
line 472 at r1 (raw file):
Previously, amosStarkware wrote…
Talking via slack to make sure I understand
Done.
crates/blockifier/src/transaction/account_transactions_test.rs
line 190 at r1 (raw file):
Previously, amosStarkware wrote…
can you add the expected message?
https://doc.rust-lang.org/rust-by-example/testing/unit_testing.html#:~:text=This%20attribute%20accepts%20optional%20parameter%20expected%20%3D%20with%20the%20text%20of%20the%20panic%20message.
Done.
crates/blockifier/src/transaction/account_transactions_test.rs
line 202 at r1 (raw file):
Previously, amosStarkware wrote…
please store this in a variable
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)
This change is